Re-enable ANN2 for setuptools #4709
Conversation
| cmd = _Command.reinitialize_command(self, command, reinit_subcommands) | ||
| vars(cmd).update(kw) | ||
| return cmd | ||
| return cmd # pyright: ignore[reportReturnType] # pypa/distutils#307 |
| outfile = str(Path(outfile).resolve()) | ||
| return super().copy_file( | ||
| outfile = str(Path(outfile).resolve()) # type: ignore[assignment] # Re-assigning a str when outfile is StrPath is ok | ||
| return super().copy_file( # pyright: ignore[reportReturnType] # pypa/distutils#309 |
| def has_magic(s): | ||
| def has_magic(s: str | bytes) -> bool: | ||
| if isinstance(s, bytes): | ||
| match = magic_check_bytes.search(s) | ||
| return magic_check_bytes.search(s) is not None | ||
| else: | ||
| match = magic_check.search(s) | ||
| return match is not None | ||
| return magic_check.search(s) is not None |
There was a problem hiding this comment.
mypy will declare the first match as typed Math[bytes] | None, so the second assignement doesn't respect that typing, this is the same issue as python/mypy#10736 AFAICT
Either the variable needs to be explicitely typed as Match[bytes] | Match[str] | None, or we simply don't assign.
setuptools/depends.py
Outdated
| def get_version(self, paths=None, default: str = "unknown"): | ||
| def get_version( | ||
| self, paths=None, default: str | int = "unknown" | ||
| ) -> str | int | None: |
There was a problem hiding this comment.
get_version, get_module_constant, and extract_constant can return Any (because of getattr on _imp.get_module and code.co_consts is typed as tuple[Any, ...]), but I'm not sure that's within intended usage ? Should that maybe be runtime validated ?
There was a problem hiding this comment.
Yeah, this module is a bit annoying...
It looks very "vestigial", not very used in the setuptools code-base itself. But is exported as part of the API.
get_module_constant and extract_constant seem to be very likely returning something compatible with the return values of ast.parse_literal and possibly other objects that can be disassembled directly from Python bytecode. But I guess Any is the best thing we can do about that.
get_version is returning None | Literal["default"] | T if self.format is defined as Callable[..., T] or Any otherwise. But I don't think it is worth to add any form of validation here, we want to avoid breaking this API (potentially it is a good candidate for deprecation or moving to a different 3rd-party package).
There was a problem hiding this comment.
Ok so essentially Any, but I changed it to at least give some hints.
Avasam
force-pushed
the
ANN2-autofixes
branch
3 times, most recently
from
63a7dd0 to
abca822
Compare
Avasam
force-pushed
the
ANN2-autofixes
branch
from
abca822 to
7a1006c
Compare
| import zipfile | ||
|
|
||
| mkpath(os.path.dirname(zip_filename), dry_run=dry_run) | ||
| mkpath(os.path.dirname(zip_filename), dry_run=dry_run) # type: ignore[arg-type] # python/mypy#18075 |
Avasam
force-pushed
the
ANN2-autofixes
branch
from
7a1006c to
3ee8f10
Compare
| infile = str(Path(infile).resolve()) | ||
| outfile = str(Path(outfile).resolve()) | ||
| return super().copy_file( | ||
| outfile = str(Path(outfile).resolve()) # type: ignore[assignment] # Re-assigning a str when outfile is StrPath is ok |
There was a problem hiding this comment.
This may or may not also be related to python/mypy#18075
setuptools/depends.py
Outdated
| def get_version(self, paths=None, default: str = "unknown"): | ||
| def get_version( | ||
| self, paths=None, default: str | int = "unknown" | ||
| ) -> str | int | None: |
There was a problem hiding this comment.
Yeah, this module is a bit annoying...
It looks very "vestigial", not very used in the setuptools code-base itself. But is exported as part of the API.
get_module_constant and extract_constant seem to be very likely returning something compatible with the return values of ast.parse_literal and possibly other objects that can be disassembled directly from Python bytecode. But I guess Any is the best thing we can do about that.
get_version is returning None | Literal["default"] | T if self.format is defined as Callable[..., T] or Any otherwise. But I don't think it is worth to add any form of validation here, we want to avoid breaking this API (potentially it is a good candidate for deprecation or moving to a different 3rd-party package).
Avasam
force-pushed
the
ANN2-autofixes
branch
from
3ee8f10 to
f160c70
Compare
| if TYPE_CHECKING: | ||
| from typing_extensions import TypeAlias | ||
|
|
||
| from pkg_resources import Distribution as _pkg_resources_Distribution |
There was a problem hiding this comment.
It is good this is inside a if TYPE_CHECKING block. Thank you!
Ideally we want to avoid importing pkg_resources in runtime because of the performance hit.
It is only acceptable to "lazily" import pkg_resources on deprecated code paths (but never eagerly).
|
I don't know if it was a false negative but apparently after merging this in top of the previous merges we have a
Maybe there is a new version of @Avasam, do you have any info? |
I can take a look at it tomorrow. I'm omw to a Halloween party 🎃 If needed for other PRs you can suppress the error temporarily (or suppress for entire file if it only happens on one side of the 3.12 distutils boundary) It's possible that two PRs individually didn't trigger new errors, but together do. And it wouldn't be caught by CI if the branches weren't updated (and there was no conflict forcing an update) |
2 participants
Summary of changes
Follow-up to #4504, works towards #2345
I didn't include long/complex overloads in this PR, that may come later
This has some overlap with #4711, but they can be merged in either order.
Pull Request Checklist
newsfragments/. (no public facing changes)(See documentation for details)